Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor GrpcStatus to implement GrpcExceptionHandlerFunction #5571

Merged
merged 32 commits into from
May 20, 2024

Conversation

jaeseung-bae
Copy link
Contributor

Motivation:

Modifications:

  • GrpcStatus implements GrpcExceptionHandlerFunction
  • Rename GrpcStatus to DefaultGrpcExceptionHandlerFunction

Result:

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 74.10%. Comparing base (14c5566) to head (866a041).
Report is 5 commits behind head on main.

Current head 866a041 differs from pull request most recent head 0cef19b

Please upload reports for the commit 0cef19b to get more accurate results.

Files Patch % Lines
...rmeria/internal/client/grpc/ArmeriaClientCall.java 83.33% 1 Missing ⚠️
...meria/internal/common/grpc/HttpStreamDeframer.java 80.00% 1 Missing ⚠️
...inecorp/armeria/server/grpc/FramedGrpcService.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5571      +/-   ##
============================================
+ Coverage     74.05%   74.10%   +0.04%     
+ Complexity    21253    21010     -243     
============================================
  Files          1850     1819      -31     
  Lines         78600    77399    -1201     
  Branches      10032     9889     -143     
============================================
- Hits          58209    57355     -854     
+ Misses        15686    15390     -296     
+ Partials       4705     4654      -51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jaeseung-bae jaeseung-bae marked this pull request as ready for review April 6, 2024 10:43
@ikhoon ikhoon added improvement sprint Issues for OSS Sprint participants labels Apr 8, 2024
Comment on lines 251 to 254
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, cause,
metadata)
.withDescription(cause.getMessage())
.asRuntimeException());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set DefaultGrpcExceptionHandlerFunction as a fallback exception handler for exceptionHandler when it is set in the GrpcClientBuilder like we do in ServerBuilder?

final ServerErrorHandler errorHandler;
if (this.errorHandler == null) {
errorHandler = ServerErrorHandler.ofDefault();
} else {
// Ensure that ServerErrorHandler never returns null by falling back to the default.
errorHandler = this.errorHandler.orElse(ServerErrorHandler.ofDefault());
}

The rest of code should not access to DefaultGrpcExceptionHandlerFunction and use the composed one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll modify to use DefaultGrpcExceptionHandlerFunction as fallback exception handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global comments) If DefaultGrpcExceptionHandlerFunction is set as the fallback, should we remove DefaultGrpcExceptionHandlerFunction here and others?

Suggested change
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, cause,
metadata)
.withDescription(cause.getMessage())
.asRuntimeException());
exceptionHandler.apply(ctx, cause, metadata)
.withDescription(cause.getMessage())
.asRuntimeException());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback. Thanks

@@ -424,7 +425,7 @@ public void onNext(DeframedMessage message) {
new Metadata());
} else {
GrpcWebTrailers.set(ctx, trailers);
GrpcStatus.reportStatus(trailers, this);
DefaultGrpcExceptionHandlerFunction.reportStatus(trailers, this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reportStatus is unrelated to GrpcExceptionHandlerFunction. Should we leave it in GrpcStatus and only move code handling exceptions to DefaultGrpcExceptionHandlerFunction?

Copy link
Contributor Author

@jaeseung-bae jaeseung-bae Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the recommendation. I also think we should separate GrpcStatus and DefaultGrpcExceptionHandlerFunction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback. Thanks

if (exceptionHandler != null) {
option(EXCEPTION_HANDLER.newValue(exceptionHandler));
if (exceptionHandler == null) {
exceptionHandler = DefaultGrpcExceptionHandlerFunction.ofDefault();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's expose DefaultGrpcExceptionHandlerFunction using GrpcExceptionHandlerFunction.ofDefault().

Suggested change
exceptionHandler = DefaultGrpcExceptionHandlerFunction.ofDefault();
exceptionHandler = GrpcExceptionHandlerFunction.ofDefault();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback. Thanks

Comment on lines 251 to 254
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, cause,
metadata)
.withDescription(cause.getMessage())
.asRuntimeException());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global comments) If DefaultGrpcExceptionHandlerFunction is set as the fallback, should we remove DefaultGrpcExceptionHandlerFunction here and others?

Suggested change
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, cause,
metadata)
.withDescription(cause.getMessage())
.asRuntimeException());
exceptionHandler.apply(ctx, cause, metadata)
.withDescription(cause.getMessage())
.asRuntimeException());

@@ -89,19 +89,24 @@ void sortExceptionHandler() {
B1Exception.class);

final GrpcExceptionHandlerFunction exceptionHandler = builder.build();
Status status = GrpcStatus.fromThrowable(exceptionHandler, ctx, new A3Exception(), new Metadata());
Status status = DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to use DefaultGrpcExceptionHandlerFunction to call the exceptionHandler.
If DefaultGrpcExceptionHandlerFunction is necessary, exceptionHandler.orElse(GrpcExceptionHandlerFunction.ofDefault() can be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback. Thanks


final ClientBuilderParams clientParams = Clients.unwrap(client, ClientBuilderParams.class);
assertThat(clientParams.options().get(GrpcClientOptions.EXCEPTION_HANDLER))
.isEqualTo(GrpcExceptionHandlerFunction.ofDefault());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isSameAs() is probably a better choice in this case because we want it to be an exactly same instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This testcase has been removed after some modification.
But there is still test case useDefaultGrpcExceptionHandlerFunctionAsFallback

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Left minor comments on testing.

ikhoon and others added 5 commits April 25, 2024 16:10
…ionHandlerFunctionBuilderTest.java

Co-authored-by: Ikhun Um <ih.pert@gmail.com>
…ionHandlerFunctionBuilderTest.java

Co-authored-by: Ikhun Um <ih.pert@gmail.com>
…ionHandlerFunctionBuilderTest.java

Co-authored-by: Ikhun Um <ih.pert@gmail.com>
…ionHandlerFunctionBuilderTest.java

Co-authored-by: Ikhun Um <ih.pert@gmail.com>
@jaeseung-bae
Copy link
Contributor Author

Overall looks good. Left minor comments on testing.

Applied feedback, Thanks.
I've checked that test is working fine after change.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thanks, @jaeseung-bae! 🙇‍♂️🚀

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically it looks great! Left few comments. 😉

if (exceptionHandler != null) {
option(EXCEPTION_HANDLER.newValue(exceptionHandler));
if (exceptionHandler == null) {
exceptionHandler = GrpcExceptionHandlerFunction.ofDefault();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that we shouldn't set the property while building an instance:

GrpcExceptionHandlerFunction exceptionHandler;
if (this.exceptionHandler == null) {
    exceptionHandler = GrpcExceptionHandlerFunction.ofDefault();
} else {
    exceptionHandler = this.exceptionHandler.orElse(GrpcExceptionHandlerFunction.ofDefault());
}
exceptionHandler = new UnwrappingGrpcExceptionHandleFunction(exceptionHandler);
option(EXCEPTION_HANDLER.newValue(exceptionHandler));

Copy link
Contributor Author

@jaeseung-bae jaeseung-bae May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, exceptionHandler is passed through ClientBuilderParams params for instantiating ArmeriaChannel. This look like a current way to pass parameters by ClientBuilderParams and so exceptionHandler should be set. I may introduce exceptionHandler as new constructor parameter but I'm not sure that is right to do. Do you have any idea on this?

ArmeriaChannel(ClientBuilderParams params,
HttpClient httpClient,
MeterRegistry meterRegistry,
SessionProtocol sessionProtocol,
SerializationFormat serializationFormat,
@Nullable GrpcJsonMarshaller jsonMarshaller,
Map<MethodDescriptor<?, ?>, String> simpleMethodNames) {
this.params = params;
this.httpClient = httpClient;
this.meterRegistry = meterRegistry;
this.sessionProtocol = sessionProtocol;
this.serializationFormat = serializationFormat;
this.jsonMarshaller = jsonMarshaller;
this.simpleMethodNames = simpleMethodNames;
final ClientOptions options = options();
maxOutboundMessageSizeBytes = options.get(GrpcClientOptions.MAX_OUTBOUND_MESSAGE_SIZE_BYTES);
maxInboundMessageSizeBytes = maxInboundMessageSizeBytes(options);
unsafeWrapResponseBuffers = options.get(GrpcClientOptions.UNSAFE_WRAP_RESPONSE_BUFFERS);
useMethodMarshaller = options.get(GrpcClientOptions.USE_METHOD_MARSHALLER);
compressor = options.get(GrpcClientOptions.COMPRESSOR);
decompressorRegistry = options.get(GrpcClientOptions.DECOMPRESSOR_REGISTRY);
credentials0 = options.get(GrpcClientOptions.CALL_CREDENTIALS);
exceptionHandler = options.get(GrpcClientOptions.EXCEPTION_HANDLER);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exceptionHandler is retrieved from options so we don't need to add exceptionHandler to the constructor parameter:

exceptionHandler = options.get(GrpcClientOptions.EXCEPTION_HANDLER);

I also realized that we have to create a new exceptionHandler only when it's set:

public <T> T build(Class<T> clientType) {
    requireNonNull(clientType, "clientType");

    final List<ClientInterceptor> clientInterceptors = interceptors.build();
    if (!clientInterceptors.isEmpty()) {
        option(INTERCEPTORS.newValue(clientInterceptors));
    }
    if (exceptionHandler != null) {
        option(EXCEPTION_HANDLER.newValue(new UnwrappingGrpcExceptionHandleFunction(
                exceptionHandler.orElse(GrpcExceptionHandlerFunction.of()))));
    }
    ....
}

public static final ClientOption<GrpcExceptionHandlerFunction> EXCEPTION_HANDLER =
        ClientOption.define("EXCEPTION_HANDLER", new UnwrappingGrpcExceptionHandleFunction(
                GrpcExceptionHandlerFunction.of()));
               

@@ -173,8 +172,7 @@ public final class GrpcClientOptions {
* to a gRPC {@link Status}.
*/
public static final ClientOption<GrpcExceptionHandlerFunction> EXCEPTION_HANDLER =
ClientOption.define("EXCEPTION_HANDLER",
(ctx, cause, metadata) -> GrpcStatus.fromThrowable(cause));
ClientOption.define("EXCEPTION_HANDLER", GrpcExceptionHandlerFunction.ofDefault());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use UnwrappingGrpcExceptionHandleFunction because this value is never used. Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnwrappingGrpcExceptionHandleFunction should be the outermost handler so we don't use it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, probably need to add a comment for that?
GrpcExceptionHandlerFunction.ofDefault() isn't used because the actual exceptionHandler is set while building a gRPC client in GrpcClientBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment, @minwoox can you check it again? Thanks,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this change, we have to use it now.
Sorry but could you revert it? 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll revert it

* Returns the default {@link GrpcExceptionHandlerFunction}.
*/
@UnstableApi
static GrpcExceptionHandlerFunction ofDefault() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recently, we just use of for the default instance.

Suggested change
static GrpcExceptionHandlerFunction ofDefault() {
static GrpcExceptionHandlerFunction of() {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback, Thanks.

private final GrpcExceptionHandlerFunction delegate;

public UnwrappingGrpcExceptionHandleFunction(GrpcExceptionHandlerFunction handlerFunction) {
this.delegate = handlerFunction;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
this.delegate = handlerFunction;
delegate = handlerFunction;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback, Thanks.

return delegate.apply(ctx, t, metadata);
}

private Throwable peelAndUnwrap(Throwable t) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
private Throwable peelAndUnwrap(Throwable t) {
private static Throwable peelAndUnwrap(Throwable t) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback, Thanks.

if (exceptionHandler == null) {
exceptionHandler = GrpcExceptionHandlerFunction.ofDefault();
} else {
exceptionHandler = exceptionHandler.orElse(GrpcExceptionHandlerFunction.ofDefault());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

@jaeseung-bae jaeseung-bae May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ditto to #5571 (comment)?
I replied to #5571 (comment)?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can probably do:

GrpcExceptionHandlerFunction grpcExceptionHandler;
if (exceptionMappingsBuilder != null) {
    grpcExceptionHandler = exceptionMappingsBuilder.build().orElse(GrpcExceptionHandlerFunction.of());
} else if (exceptionHandler != null) {
    grpcExceptionHandler = exceptionHandler.orElse(GrpcExceptionHandlerFunction.of());;
} else {
    grpcExceptionHandler = GrpcExceptionHandlerFunction.of();
}
grpcExceptionHandler = new UnwrappingGrpcExceptionHandleFunction(grpcExceptionHandler);
registryBuilder.setDefaultExceptionHandler(grpcExceptionHandler);

if (interceptors != null) {
    final HandlerRegistry.Builder newRegistryBuilder = new HandlerRegistry.Builder();
    final ImmutableList<ServerInterceptor> interceptors = this.interceptors.build();
    for (Entry entry : registryBuilder.entries()) {
        final MethodDescriptor<?, ?> methodDescriptor = entry.method();
        final ServerServiceDefinition intercepted =
                ServerInterceptors.intercept(entry.service(), interceptors);
        newRegistryBuilder.addService(entry.path(), intercepted, methodDescriptor, entry.type(),
                                      entry.additionalDecorators());
    }
    newRegistryBuilder.setDefaultExceptionHandler(grpcExceptionHandler);
    handlerRegistry = newRegistryBuilder.build();
} else {
    handlerRegistry = registryBuilder.build();
}

Copy link
Contributor Author

@jaeseung-bae jaeseung-bae May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied feedback, Thank.
Code gets cleaner :)

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically it looks great! Left a few comments. 😉

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once @minwoox's comments are addressed. Thanks, @jaeseung-bae!

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good once @minwoox 's comments are addressed 👍 👍 👍

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two nits. 😉
Thanks a lot, @jaeseung-bae!

jaeseung-bae and others added 2 commits May 20, 2024 11:03
…Options.java

Co-authored-by: minux <songmw725@gmail.com>
…nwrappingGrpcExceptionHandleFunction.java

Co-authored-by: minux <songmw725@gmail.com>
@minwoox minwoox merged commit 1e14396 into line:main May 20, 2024
14 of 15 checks passed
@minwoox
Copy link
Member

minwoox commented May 20, 2024

@jaeseung-bae, 👍 👍 👍

Dogacel pushed a commit to Dogacel/armeria that referenced this pull request Jun 8, 2024
)

Motivation:

- Closes line#5550 

Modifications:

- `GrpcStatus` implements `GrpcExceptionHandlerFunction`
- Rename `GrpcStatus` to `DefaultGrpcExceptionHandlerFunction`

Result:

- Closes line#5550
- GrpcClientBuilder uses `DefaultGrpcExceptionHandlerFunction` as default
- GrpcService can use `DefaultGrpcExceptionHandlerFunction` for its `exceptionHandler`

<!--
Visit this URL to learn more about how to write a pull request description:
https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement sprint Issues for OSS Sprint participants
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make GrpcStatus implement GrpcExceptionHandlerFunction
5 participants